Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit private callback lookup. #58

Merged
merged 1 commit into from
Jan 19, 2013
Merged

Limit private callback lookup. #58

merged 1 commit into from
Jan 19, 2013

Conversation

svoop
Copy link

@svoop svoop commented Dec 15, 2012

Support for private callbacks introduced in commit 71937c0 is a good thing, however, the way private callbacks are looked up caused a nasty issue in one of my models dealing with credit cards:

class CreditcardPayment < ActiveRecord::Base
  workflow do
    state :pending do
      (...)
      event :fail, transitions_to: :failed
    end
    state :failed
  end
end

Now try to transition with fail! and - kaboom - runtime error. The problem is the lookup in the workflow gem:

def has_callback?(action)
  self.respond_to?(action) or self.class.private_method_defined?(action)
end

Eventhough no private event handler fail is defined in the model, private_method_defined? still returns true as it does for any object due to Kernel#fail.

The problem is quite nasty and since "fail" appears to be a rather common transition in workflows, using another event name is not the best solution. But there's an easy way to limit the method lookup:

def has_callback?(action)
  self.respond_to?(action) or self.private_methods(false).include?(action)
end

Which is exactly what this pull request contains :-)

@dwbutler
Copy link

Thanks for catching that! This is a good idea, but this code won't work in Ruby 1.8.7 because private_methods returns an array of strings.

#1.9.3
"hello".private_methods(false)
 => [:initialize, :initialize_copy]

#1.8.7
"hello".private_methods(false)
=> ["initialize_copy", "initialize"]

In a previous commit I ended up mapping the return value to an array of symbols.

Would be good to see some tests included! =)

Background: Commit 71937c0 introduced private callbacks, however, the use of "private_method_defined?" returns true all the way down the class hierarchy and thus causes trouble with a very common transition: Given an event "event :fail transitions_to :failed". If you don't explicitly define the "fail" event handler, "private_method_defined?" will still return true since Kernel#fail is defined for any object. Transitioning with "fail!" then causes a runtime error.
@svoop
Copy link
Author

svoop commented Dec 15, 2012

As you suggested, I've added map for Ruby 1.8.7 compatibility (can't wait for this Ruby generation to be retired next spring) and a test - all collapsed into one commit.

@dwbutler
Copy link

@geekq, think we can get a bugfix 0.8.7 release soon? =)

@geekq
Copy link
Owner

geekq commented Jan 10, 2013

Sorry - was busy with my current project.

  • I am going to switch the gem releasing away from jeweller to the modern pure bundler as I did for jetty-rackup
  • Will apply all the piled up patches and release then

Going to do this this weekend/monday ETA: 2013-01-14

@geekq
Copy link
Owner

geekq commented Jan 16, 2013

The change will brake class hierarchies with private callback methods. But it is less harm, than unintentionally calling some private methods, especially in kernel. So going to merge... Would like to test with ruby-1.9.3, ruby-1.8.7 and jruby. Unfortunately my current rbenv for 1.8.7 and 1.8.6 broken. Need to repair that first.

@dwbutler
Copy link

Wouldn't defining the methods as protected instead of private allow them to be called in a child class? (I'm not entirely sure - I'd have to do some testing to determine if respond_to? returns true for protected methods.)

@dwbutler
Copy link

After some testing I've determined the following about protected methods defined on a parent class: (this is consistent in 1.9.3 and 1.8.7):

  • respond_to? returns false.
  • protected_instance_methods includes only the protected methods, and no core Ruby methods.
  • protected_method_defined? returns true

Based on these results, I say we add protected_method_defined? into the conditions and recommend people to use protected rather than private methods.

@dwbutler
Copy link

I made a gist to help me understand and document all the craziness behind private methods, protected methods, and inheritance:

https://gist.github.com/4554269

@geekq geekq merged commit 411ddab into geekq:master Jan 19, 2013
@geekq
Copy link
Owner

geekq commented Jan 19, 2013

Hi,

I've prepared my continuous integration environment for testing with various ruby versions. Not yet on travis-ci.org, just on my notebook. Can now review/try out/merge.

@sven Thanks for the test and for the improved 1.8.7 compatibility!

@david Thanks for the 1.8.7 investigation and for the 'protected method' idea!

I think, using protected methods as callbacks is the best way to go:

  • callbacks can be hidden (non public) as desired in Support for private callbacks #53
  • no unintentional calls on fail! and other Kernel methods
  • inheritance hierarchy with workflow is still supported

Best Regards,

Vladimir (geekq)

@dwbutler
Copy link

Thanks!

My idea was actually to support both private and protected methods, by using both private_instance_methods(false).map(&:to_sym).include? and protected_method_defined?. Do you think it's feasible to support both? I think a lot of Rubyists are used to using private rather than protected methods.

@geekq
Copy link
Owner

geekq commented Jan 20, 2013

OK, adding support for private methods again ;-) - but only in the immediate class now.
Also expanding tests.

Big surprise: respond_to? returns true for inherited protected methods (for me). But it can change one more time in Ruby2.

Meta programming is hard and it is even harder with a moving target like Ruby 1.9 ;-)

So I'll keep all the 3 checks even if the second one currently seems to be unneeded on 1.8.7-p371 and 1.9.3-rc1

@dwbutler
Copy link

Great! I like how to checks are documented now with some comments! =D Otherwise it would be quite confusing to understand why all those checks are necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants